QUALITY-544: Lock NLD for image-attached agent-view entry#9366
QUALITY-544: Lock NLD for image-attached agent-view entry#9366evelyn-with-warp wants to merge 3 commits intomasterfrom
Conversation
When the user pastes or drags-and-drops an image into a terminal that has NLD on with autodetection enabled, entering agent view should lock the input to AI mode for the duration of the attach. Previously, two leak sites would unlock the input and let the classifier flip the buffer back to shell mode after the entry: 1. The `EnteredAgentView` subscriber in `BlocklistAIInputModel` would set `is_locked: !is_ai_autodetection_enabled` even for `ImageAdded` origins. 2. `set_input_mode_agent`'s `should_unlock` branch (which is intended for the `!`-shell-toggle clear path in fullscreen agent view with empty buffer) would re-unlock immediately after the image-add path entered agent view. This change introduces a single `has_locking_attachment()` predicate on `BlocklistAIContextModel` and uses it to gate both leak sites. It also adds an image-attachment-in-progress counter so the predicate is true during the async file-read + resize/encode pipeline, before the final `ImageContext` is appended to `pending_attachments`. Changes: - Add `pending_image_attachments_in_progress: usize` and `has_locking_attachment()` / `note_image_attachment_started()` / `note_image_attachment_completed()` to `BlocklistAIContextModel`. - Wire a handle to `BlocklistAIContextModel` into `BlocklistAIInputModel` via a `set_ai_context_model` setter, called from `TerminalView::new` after both models exist. - Add an `origin_requires_locked_ai` helper and force-lock to AI in the `EnteredAgentView` subscriber when the origin is `ImageAdded` or when `has_locking_attachment()` is true. - Defense in depth: gate `BlocklistAIInputModel::is_autodetection_enabled_for_current_context` on `!has_locking_attachment()`. - Increment / decrement the in-progress counter synchronously in `read_and_process_images_async` and `process_and_attach_images_as_ai_context` so callers see `has_locking_attachment() == true` for the entire attach pipeline. - Reorder `handle_pasted_or_dragdropped_image_filepaths` and `process_and_attach_clipboard_image` so the editor's image dispatch runs before `set_input_mode_agent`, ensuring the counter is already incremented when the lock decision is made. - Gate `set_input_mode_agent`'s `should_unlock` on `!has_locking_attachment()`. Co-Authored-By: Oz <oz-agent@warp.dev>
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @evehsu on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
I'm starting a first review of this pull request. I completed the review and posted feedback on this pull request. Comment I completed the review and posted feedback on this pull request. Comment I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and posted feedback on this pull request. Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR locks agent-view input to AI mode when image attachment is in progress or pending image/block/selection context exists, and wires the input model to the context model so NLD can be bypassed for those cases.
Concerns
- The new image-processing in-progress counter can leak when the processing future is aborted during submit, leaving
has_locking_attachment()true until a broader context reset.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| // `note_image_attachment_completed` in the spawn callback below. | ||
| if let Some(context_model) = &self.context_model { | ||
| context_model.update(ctx, |context_model, ctx| { | ||
| context_model.note_image_attachment_started(ctx); |
There was a problem hiding this comment.
abort_attached_images_future_handle on submit, note_image_attachment_completed never runs and pending_image_attachments_in_progress stays nonzero; pair the abort path with a decrement or ensure completion always runs.
There was a problem hiding this comment.
Overview
This PR adds a pending image-attachment counter and uses it, plus existing pending context, to force-lock agent input to AI mode while images are being attached and to bypass NLD for image-driven agent-view entry.
Concerns
- The new counter is not decremented when image processing is aborted, which can leave
has_locking_attachment()true after a submit/abort race and keep input force-locked until a broader context reset. - Security pass: no security-specific findings in the inlined diff.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| }); | ||
| } | ||
|
|
||
| self.process_attached_images_future_handle = Some(ctx.spawn( |
There was a problem hiding this comment.
submit_ai_query calls abort_attached_images_future_handle, and ctx.spawn's default abort callback is a no-op, so an aborted image-processing future leaves pending_image_attachments_in_progress nonzero and keeps the input force-locked until a broader context reset. Use spawn_abortable or decrement in the explicit abort handler.
Cover `has_locking_attachment` (default false; true on pending block ids,
selected text, or pending image; ignores file-only attachments; true while
the in-progress image-attach counter is non-zero) and the
`note_image_attachment_started` / `note_image_attachment_completed` counter
mechanics including saturating decrement at zero and multiple concurrent
in-flight pipelines.
To keep the test fixture small, add `#[cfg(test)]` `new_for_test`
constructors on `BlocklistAIContextModel` and `AmbientAgentViewModel` that
skip subscriptions/singleton lookups (`BlocklistAIHistoryModel`,
`LLMPreferences`, `CloudModel`, `UpdateManager`, `AppExecutionMode`). This
follows the existing `new_for_test` convention used by `Sessions`,
`TerminalModel`, `AuthManager`, `BlocklistAIHistoryModel`, etc.
Tests live in `app/src/ai/blocklist/context_model_test.rs` per the repo's
`${filename}_tests.rs` / `mod_test.rs` convention.
Co-Authored-By: Oz <oz-agent@warp.dev>
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @evehsu on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
Switch from ctx.spawn to ctx.spawn_abortable in process_and_attach_images_as_ai_context so pending_image_attachments_in_progress is decremented on every exit path: - on_resolve (normal completion): unchanged - on_resolve early-return when handle was taken between value production and dispatch: now decrements before returning instead of leaking - on_abort (future aborted before producing a value, e.g. submit_ai_query calling abort_attached_images_future_handle while processing was in flight): new explicit handler that decrements Previously ctx.spawn's default no-op on_abort callback meant that aborting the spawned future left pending_image_attachments_in_progress nonzero, keeping has_locking_attachment() true and force-locking the input into AI mode until an unrelated context reset. Co-Authored-By: Oz <oz-agent@warp.dev>
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @evehsu on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
|
/oz-review |
There was a problem hiding this comment.
Overview
This PR adds a pending image-attachment counter to the blocklist AI context model, wires the input model to consult that locking context, and reorders image attach flows so AI mode is locked before NLD can run. It also adds unit coverage for the new context-model locking predicates and counter behavior.
Concerns
- No blocking correctness or security concerns found in the inlined diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
Description
QUALITY-544: Entering agent view with image attached
Testing
Cargo commands
Agent Mode
Changelog Entries for Stable
CHANGELOG-BUG-FIX: if user attaches an image in block input we should lock in agent mode, without running the NLD classifier to remove uncertainty